Skip to content

add Darwin family systems handling for totalSystemMemory #24903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 25, 2025
Merged

add Darwin family systems handling for totalSystemMemory #24903

merged 3 commits into from
Aug 25, 2025

Conversation

imomaliev
Copy link
Contributor

@imomaliev imomaliev commented Aug 18, 2025 •
edited
Loading

Hi, I just started learing zig. I was trying to fetch total system memory on macOS and noticed that totalSystemMemory does not support it.

This PR adds it.

I believe there are a few issues with totalSystemMemory for .freebsd and std.posix.sysctlbynameZ, but I do not know what the correct approach to take here

  1. totalSystemMemory for .freebsd returns usize instead of u64, I believe this should be fixed
  2. totalSystemMemory for .freebsd does not handle all the errors from std.posix.sysctlbynameZ.
  3. Last but not least, std.posix.sysctlbynameZ never returns NameTooLong. So either we should change the return type, or, as @andrewrk did in 3640303 we should add name_len handling. I did not find any mention of ENAMETOOLONG in manpages for freebsd/posix. So I am having a hard time understanding which way is correct here.

Just for the context I know that there were discussions in #16350 that resulted in the rollback of patches that already implemented this functionality before. Please let me know what should be done to meet contributing standards to avoid same fate as previous contributor 🙂

I am ready to provide patches for 1 and 2 in this PR or create separate one

Thanks!

@alexrp alexrp self-assigned this Aug 18, 2025
@imomaliev imomaliev force-pushed the feat/totalsystemmemory-macos branch from 41a5088 to 9492937 Compare August 18, 2025 18:20
@imomaliev imomaliev changed the title add macOS switch for totalSystemMemory add macOS handling for totalSystemMemory Aug 18, 2025
rohlem
rohlem reviewed Aug 18, 2025
View reviewed changes
@alexrp
Copy link
Member

alexrp commented Aug 18, 2025

totalSystemMemory for .freebsd returns usize instead of u64, I believe this should be fixed

u64 looks right to me.

totalSystemMemory for .freebsd does not handle all the errors from std.posix.sysctlbynameZ.

This doesn't matter too much since I'll get to it when I do #6389 anyway. But yes, it probably should.

Last but not least, std.posix.sysctlbynameZ never returns NameTooLong. So either we should change the return type, or, as @andrewrk did in 3640303 we should add name_len handling. I did not find any mention of ENAMETOOLONG in manpages for freebsd/posix. So I am having a hard time understanding which way is correct here.

I think just remove the dead NameTooLong prong.

@imomaliev imomaliev force-pushed the feat/totalsystemmemory-macos branch from 469c895 to 243ced2 Compare August 19, 2025 12:41
@imomaliev imomaliev mentioned this pull request Aug 19, 2025
Merged
@imomaliev
Copy link
Contributor Author

@alexrp Thanks. Fixed 1, but decided to fix 3 in separate PR #24909

alexrp
alexrp reviewed Aug 19, 2025
View reviewed changes
@imomaliev
Copy link
Contributor Author

@alexrp Thanks for the review! If there are no more questions/comments left, I believe this PR is ready for merging

@SeanTheGleaming
Copy link
Contributor

Could this also apply to the rest of the Darwin family? (.driverkit, .ios, .macos, .tvos, .visionos, .watchos)

@imomaliev
Copy link
Contributor Author

@SeanTheGleaming Hi! Do you know if all of them support sysctlbyname?

@SeanTheGleaming
Copy link
Contributor

@SeanTheGleaming Hi! Do you know if all of them support sysctlbyname?

I do not know for sure, but based on this article and the fact that many apis like this are shared throughout Darwin, I would assume so.

@alexrp
Copy link
Member

alexrp commented Aug 24, 2025

Could this also apply to the rest of the Darwin family? (.driverkit, .ios, .macos, .tvos, .visionos, .watchos)

This is the case more often than not. I see no indication that this particular function would only work on macOS.

@imomaliev imomaliev force-pushed the feat/totalsystemmemory-macos branch from 243ced2 to 0ca339d Compare August 25, 2025 12:29
@imomaliev
Copy link
Contributor Author

Thanks, added. @alexrp Is there any particular order for the Target switch cases? I've put Darwin family handling under .freebsd, but maybe I should put it somewhere else?

@alexrp
Copy link
Member

alexrp commented Aug 25, 2025

No, the order doesn't particularly matter.

@alexrp alexrp enabled auto-merge (squash) August 25, 2025 16:01
@imomaliev imomaliev changed the title add macOS handling for totalSystemMemory add Darwin family systems handling for totalSystemMemory Aug 25, 2025
@alexrp alexrp merged commit 91040b5 into ziglang:master Aug 25, 2025
14 checks passed
alexrp pushed a commit that referenced this pull request Aug 25, 2025
@imomaliev @alexrp
add macOS handling for totalSystemMemory (#24903)
6f07cc9 
* add macos handling for totalSystemMemory

* fix return type cast for .freebsd in totalSystemMemory

* add handling for the whole Darwin family in totalSystemMemory
@imomaliev imomaliev deleted the feat/totalsystemmemory-macos branch August 26, 2025 08:27
@alexrp alexrp removed their assignment Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrp alexrp alexrp left review comments

+1 more reviewer

@rohlem rohlem rohlem left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@imomaliev @alexrp @SeanTheGleaming @rohlem